Skip to content

Conversation

ym
Copy link
Contributor

@ym ym commented Mar 26, 2025

No description provided.

@ym ym force-pushed the feat/metrics-optin branch from 6ffc220 to f4e29f1 Compare March 26, 2025 23:52
@SuperQ
Copy link
Contributor

SuperQ commented Mar 28, 2025

Please do not do this. There is no reason to disable the metrics endpoint by default.

@ym ym marked this pull request as draft March 28, 2025 10:01
@ym
Copy link
Contributor Author

ym commented Mar 28, 2025

Please do not do this. There is no reason to disable the metrics endpoint by default.

The metrics endpoint is currently unauthenticated, and please don't get me wrong, I have no intention of disabling or removing it, the goal is to make it more secure. Right now it's a security risk that we can't ignore as we continue to add more metrics. But for now, I'm going to move this PR to draft since the UI and password part is not yet finished.

@SuperQ
Copy link
Contributor

SuperQ commented Mar 28, 2025

Sorry, but your assumption is incorrect. There is no security risk with metrics.

There is nothing "more secure" by disabling metrics.

If there is a security issue, please describe it in detail. Otherwise you are making statements without basis.

@IDisposable
Copy link
Contributor

Circling back to this, I am beginning to see the metrics GATHERING consume a significant slice of the device's CPU...we might be well served to allow folks to opt into (or out of) that overhead.

@SuperQ
Copy link
Contributor

SuperQ commented Aug 25, 2025

@IDisposable Do you have a pprof output that I can see? Gathering should be very low overhead. It would be interesting to know what's going on.

Also, Gathering metrics only happens when a user is using metrics. So, opting out would disable intentionally gathered metrics. That doesn't make a lot of sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants